Traffic-Based App Rewards: Integrate the Reward Computation Trigger with the necessary daml contracts#5119
Conversation
2da39fa to
9a37516
Compare
93367ef to
fd731a7
Compare
f1fc9f1 to
eb9c129
Compare
5b32f44 to
68c9cb7
Compare
9a37516 to
0a01ed4
Compare
6074b07 to
e13324c
Compare
0a01ed4 to
22f4aad
Compare
e13324c to
b66e52e
Compare
6981b33 to
eba9097
Compare
|
@rautenrieth-da, this is the PR that integrates the RewardComputationTrigger into this feature branch. PTAL |
rautenrieth-da
left a comment
There was a problem hiding this comment.
This trigger tries to run the reward computation for each round, starting from the earliest round with the rewardConfig set. This results in a gapless history of reward data, but can potentially lead to a lot of lag and useless computation when a SV node is catching up.
We can change the whole logic for what round to process to a simple "Fetch the oldest active CalculateRewardsV2 contract, and calculate the root hash its round".
Why does this work?
- We do not need a gapless history for the reward data. We only need it for processing rewards. Once all rewards are processed, the data will still be persisted in the UpdateHistory of
CalculateRewardsV2andProcessRewardsV2contracts. We therefore don't expect to ever need to make the internal scan API for reward processing public. - The reward processing trigger may read data from other scan instances if its own scan instance doesn't have the data, so we don't need to implement any backfilling:
- When turning a
CalculateRewardsV2into aProcessRewardsV2, we need to make sure the voting on the root hash is BFT safe. If the round is after the completeness boundary of an SV, the SV should use its own scan data. If the round is before (e.g., because the SV has just onboarded or bumped their store version), the SV will do a BFT read from other SVs to avoid deadlocking the consensus. - When breaking down
ProcessRewardsV2contracts, each SV may read reward data from any single other scan instead of its own. There is no BFT read required - since we have already voted on the root hash, theProcessRewardsV2_ProcessBatchchoice would immediately fail if called with malicious data. We would only have a problem if ALL nodes that computed the root hash went offline before the processing of rewards is finished.
- When turning a
This is all very complex, let's discuss this in our next call.
79358d5 to
c39e5af
Compare
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
…undByNumber Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Divam <dfordivam@gmail.com>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
…active Signed-off-by: Tim Emiola <adetokunbo@emio.la>
eba9097 to
b8e15a0
Compare
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
b8e15a0 to
ff8020c
Compare
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
@dfordivam , @rautenrieth-da this the trigger is now simplified as discussed, PTAL |
| // For each eligible round until the configured parallelism limit, look | ||
| // up OpenMiningRound and extract inputs. The trigger will handle the | ||
| // remainder in subsequent polls. | ||
| tasks <- Future.traverse(eligible.take(context.config.parallelism)) { roundNumber => |
There was a problem hiding this comment.
why did we add parallelism here. This seems to be fairly simple DB lookup for task creation.
Also I think MonadUtil.parTraverseWithLimit is the correct API to use
There was a problem hiding this comment.
IIUC parTraverseWithLimit will compute everything in parallel? We really only want the first n of the parallelism limit. I've actually pushed another commit with a fix that adds a helper to address this PTAL
As to why we add parallelism here, this emulating how the parallelism config is used in ExpiresRewardsCouponsTrigger, with a similar idea: to avoid contention in any scenario where there are large number of potential tasks.
| .map(_.exists(_ >= task.roundNumber)) | ||
| rewardsReferenceStore.multiDomainAcsStore | ||
| .lookupContractById(CalculateRewardsV2.COMPANION)(task.calculateRewardsId) | ||
| .map(_.isEmpty) |
There was a problem hiding this comment.
missing check for already computed totals for this round
There was a problem hiding this comment.
If that check happens when the tasks are filtered (as it does), does it also need to happen here?
There was a problem hiding this comment.
this would be called only when completeTask fails, before doing the retry. If completeTask fails ever happened after total were actually calculated, then completeTask would be called again and it would fail again, resulting in a loop. This is my understanding, but I will let @rautenrieth-da to also provide clarifications on this.
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Fixes #4383
Fixes #4570
This integrates the RewardComputationTrigger with the Daml contracts from which it obtains the values it uses in its calculations
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./lsu_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines